Skip to content

ENH: Parachute trigger doesn't work if "Apogee" is used instead of "apogee"#489

Merged
Gui-FernandesBR merged 5 commits intodevelopfrom
enh/case-sensitive-triggers
Nov 30, 2023
Merged

ENH: Parachute trigger doesn't work if "Apogee" is used instead of "apogee"#489
Gui-FernandesBR merged 5 commits intodevelopfrom
enh/case-sensitive-triggers

Conversation

@Gui-FernandesBR
Copy link
Copy Markdown
Member

@Gui-FernandesBR Gui-FernandesBR commented Nov 28, 2023 •
edited
Loading

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

  • The parachute trigger option "apogee" is case sensitive, so the triggerfunc is not set if you use "Apogee" or "APOGEE" instead of the expoected "apogee" value.
  • Worse is that the Parachute class doesn't raise any error is the user pass a string like "Apogee" or "AnyOtherThing", so the user just gets an error message when simulating the Flight class

New behavior

  • Refactor the Parachute class init just a bit.
  • Using the built-in lower() method to compare the string without being case-sensitive.
  • You're gonna know if the triggerfunc cannot be set after initializing the Parachute class.

Breaking change

  • No

Additional information

Reported by jackson.dendy on discord tonight, from the univ of Tenesse.

@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes Parachute Related to parachutes methods and usage labels Nov 28, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Nov 28, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Nov 28, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0ef9849) 70.91% compared to head (c1e43ec) 70.90%.
Report is 11 commits behind head on develop.

❗ Current head c1e43ec differs from pull request most recent head a6dfea3. Consider uploading reports for the commit a6dfea3 to get more accurate results

Files Patch % Lines
rocketpy/rocket/parachute.py 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #489      +/-   ##
===========================================
- Coverage    70.91%   70.90%   -0.01%     
===========================================
  Files           55       55              
  Lines         9261     9263       +2     
===========================================
+ Hits          6567     6568       +1     
- Misses        2694     2695       +1
Flag Coverage Δ
unittests 70.90% <75.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

giovaniceotto
giovaniceotto approved these changes Nov 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix.

Improvement suggestion: line 129 of rocketpy/rocket/parachute.py

- The string "apogee," which triggers the parachute at apogee, i.e.,  when the rocket reaches its highest point and starts descending.

I believe we should change from "apogee," to "apogee",.

@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner November 30, 2023 05:22
@Gui-FernandesBR Gui-FernandesBR merged commit ae45abe into develop Nov 30, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/case-sensitive-triggers branch November 30, 2023 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giovaniceotto giovaniceotto giovaniceotto approved these changes

@MateusStano MateusStano MateusStano approved these changes

@phmbressan phmbressan Awaiting requested review from phmbressan

Assignees

@Gui-FernandesBR Gui-FernandesBR

Labels

Enhancement New feature or request, including adjustments in current codes Parachute Related to parachutes methods and usage

Projects

No open projects
LibDev Roadmap
Status: Closed

Milestone

Release v1.X.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Gui-FernandesBR @giovaniceotto @MateusStano